Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[HOTFIX][MI100][OCL] Remove unsed kernel arg for wrw fp32 NCHW #1013

Merged
merged 7 commits into from
Jun 30, 2021
Merged

[HOTFIX][MI100][OCL] Remove unsed kernel arg for wrw fp32 NCHW #1013

merged 7 commits into from
Jun 30, 2021

Conversation

carlushuang
Copy link
Contributor

This resolve issue #1012, when under fp32 wrw, it seems feed in more kernel arg than needed. This seems OK under HIP, but in OCL it will report issue. But anyway we need keep consistent kernel arg with metadata declaration.

@shaojiewang please double check this fp32 wrw

@carlushuang carlushuang linked an issue Jun 26, 2021 that may be closed by this pull request
ConvAsmImplicitGemmGTCDynamicWrwXdlops: "FAILED ... oclkernel.hpp:109: Error setting argument #19 to kernel" #1012
Closed
@carlushuang carlushuang mentioned this pull request Jun 26, 2021
Closed
shaojiewang
shaojiewang previously approved these changes Jun 26, 2021
View reviewed changes
atamazov
atamazov suggested changes Jun 26, 2021
View reviewed changes
Copy link
Contributor

@atamazov atamazov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for quick response. Constness of buffers also needs to be fixed, see #1012 (comment).

What about regression tests?

@atamazov
Copy link
Contributor

This seems OK under HIP.

I think this is not ok, #1012 (comment)

@carlushuang
Copy link
Contributor Author

carlushuang commented Jun 26, 2021 •
edited
Loading

Constness of buffers also needs to be fixed, see #1012 (comment).

OK, let me add that part into this PR (267f86e)

What about regression tests?

I guess this fix does not introduce regression? If can't work (e.g. after this PR fail on HIP), the CI will just fail

@atamazov
Copy link
Contributor

@carlushuang

What about regression tests?

I guess this fix does not introduce regression? If can't work (e.g. after this PR fail on HIP), the CI will just fail

Or course, not. But this PR should add some regression tests that should prevent this bug in the future (see #1012). If you need assistance, please let me know and I'll add them to this branch.

@carlushuang
Copy link
Contributor Author

@atamazov ok sure, I think need your help on adding some tests in this branch, in case I'm not quite clear of the regression of this PR.
Thanks :)

@codecov

This comment has been minimized.

Sign in to view
@atamazov
Copy link
Contributor

@carlushuang

@atamazov ok sure, I think need your help on adding some tests in this branch, in case I'm not quite clear of the regression of this PR. Thanks :)

Done in b6a3ff7. I would like to make this commit absolutely clear for you before merging this. Please review this commit and compare it with the description of #1012. Ask if something is unclear.

@carlushuang
Copy link
Contributor Author

carlushuang commented Jun 30, 2021 •
edited
Loading

@atamazov the flag --disable-validation is added, because we don't care about the result is correct or not, but if the test itself will fail or not (because of extra kernel arg) right?

@atamazov
Copy link
Contributor

@carlushuang Yes, we do not care about precision in this test. Extra kernel arg will cause the test to fail.

@atamazov
Copy link
Contributor

We didn't have a MI100 test for OpenCL backend, so the test weren't run. dca7901 resolves this.

atamazov
atamazov previously approved these changes Jun 30, 2021
View reviewed changes
@atamazov
Merge branch 'develop' into fix_issue_1012
be03ef0 
# RESOLVED Conflicts:
#	test/CMakeLists.txt
@atamazov atamazov changed the title [HOTFIX] remove unsed kernel arg for wrw fp32 NCHW [HOTFIX][MI100][OCL] Remove unsed kernel arg for wrw fp32 NCHW Jun 30, 2021
@atamazov atamazov merged commit c812a33 into develop Jun 30, 2021
atamazov
atamazov reviewed Jul 1, 2021
View reviewed changes
test/CMakeLists.txt
Comment on lines +1173 to +1181
set(ENVS_REGRESSION_ISSUE_1012
MIOPEN_DEBUG_IMPLICIT_GEMM_FIND_ALL_SOLUTIONS=1
MIOPEN_FIND_MODE=normal)

set(ARGS_REGRESSION_ISSUE_1012
--verbose
--disable-forward
--disable-backward-data
--disable-validation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚓

@atamazov atamazov deleted the fix_issue_1012 branch July 6, 2021 10:18
atamazov added a commit that referenced this pull request Jul 22, 2021
@carlushuang @atamazov
[HOTFIX][MI100][OCL] Remove unsed kernel arg for wrw fp32 NCHW (#1013)
70db293 
* Remove unused kernel arg for wrw fp32 xdlops NCHW
* ConvAsmImplicitGemmGTCDynamicWrwXdlops: fix metadata in igemm_wrw_gtc_gfx908.s
* Add regression tests
* Re-target OpenCL smoke tests from Vega to gfx908. Enable regression test during Smoke test stages.

Co-authored-by: Artem Tamazov <artem.tamazov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atamazov atamazov atamazov approved these changes

@shaojiewang shaojiewang shaojiewang left review comments

@junliume junliume Awaiting requested review from junliume junliume is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ConvAsmImplicitGemmGTCDynamicWrwXdlops: "FAILED ... oclkernel.hpp:109: Error setting argument #19 to kernel"
3 participants
@carlushuang @atamazov @shaojiewang